Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sistent Card Component #6117

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Anand-Theertha
Copy link
Contributor

Description

This PR fixes #5908. This PR adds a Card component to the Sistent components. Following are a few screenshots of how the implementation looks:

image

image

image

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@l5io
Copy link
Contributor

l5io commented Dec 9, 2024

🚀 Preview for commit 33df114 at: https://6756a2a4e25791afc5b16274--layer5.netlify.app

Signed-off-by: Anand-Theertha <[email protected]>
Signed-off-by: Anand-Theertha <[email protected]>
@l5io
Copy link
Contributor

l5io commented Dec 9, 2024

🚀 Preview for commit ade40a3 at: https://6756ad5a8a59a4c1a351345a--layer5.netlify.app

@leecalcote
Copy link
Member

Thank you, @Anand-Theertha 👍

Copy link
Contributor

@Vidit-Kushwaha Vidit-Kushwaha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Anand-Theertha I have reviewed your PR I have put some comments; you can look into it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text on the card is not properly visible Can you use other shades?

image

import { useStyledDarkMode } from "../../../../../theme/app/useStyledDarkMode";

const codes = [
`const cardOutlined = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than creating a new component, you can directly pass as a child component to Card. So that I will be easier to copy for developers in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it this way to keep the code modular and abstract to make it more reusable. Do you still suggest I add this as a child component? @Vidit-Kushwaha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same increase in contrast between the text and background.

@Anand-Theertha
Copy link
Contributor Author

Will make these changes, thanks @Vidit-Kushwaha

@Vidit-Kushwaha
Copy link
Contributor

Hi @Anand-Theertha Facing issues merging your PR in Sistent?

We are conducting a special tutorial on this Monday, December 16, 2024, at 7 AM Central (6:30 PM IST) in the website meeting.

We'll cover:

  • Navigating the Sistent repo
  • Setting up locally
  • Helping your initial pull request to be merged

Get more information: here

👥 Get involved:

Don’t miss out. 🚀

@sudhanshutech
Copy link
Member

@Anand-Theertha did you made changes based on feedback yet?

@sudhanshutech
Copy link
Member

@Anand-Theertha checking in again?

@Anand-Theertha
Copy link
Contributor Author

Will push these changes today, thanks for the reminder @sudhanshutech

@sudhanshutech
Copy link
Member

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[Sistent] Add Card component to the sistent components page
5 participants